Skip to content

(s)kmean setMeans is now setting the number of clusters right - #349

Open
tremblap wants to merge 1 commit into
flucoma:mainfrom
tremblap:fix/setting-centroids
Open

tremblap wants to merge 1 commit into
flucoma:mainfrom
tremblap:fix/setting-centroids

Conversation

@tremblap

Copy link
Copy Markdown
Member

fixes #120

I removed the nonsensical condition, and added 2 asserts to make sure we get the right values passed, assuming that a dataset with at least one point will have the right type. Maybe @weefuzzy will want to change the checking - I also thought of checking that the passed info was a vector of float instead of the asserts, and maybe that would be better. I'm opened to suggestions.

@tremblap
tremblap requested a review from weefuzzy October 23, 2026 08:44
@tremblap

Copy link
Copy Markdown
Member Author

example code to check in SC:

// make and plot a dataset with 5 globs
(
fork{defer{
	var counter = 0;
	var dict = Dictionary.newFrom(["cols",2,"data",Dictionary.new]);
	var xybuf = Buffer.alloc(s,2);

	1000.do{
		arg i;
		var x = [-0.7,0.7,0,0.7,-0.7][i % 5];
		var y = [-0.7,0.7,0,-0.7,0.7][i % 5];
		x = gauss(x,0.1).clip(-1,1);
		y = gauss(y,0.1).clip(-1,1);
		dict["data"][i] = [x,y];
	};

	~mean_seeds = FluidDataSet(s);

	~fp = FluidPlotter(bounds:Rect(0,0,800,800), xmin: -1, ymin: -1, dict:dict,mouseMoveAction:{
		arg view, x, y ...a;
		a[2] !? {
			xybuf.setn(0,[x,y]);
			~mean_seeds.addPoint(counter,xybuf,{
				counter = counter + 1;
				~mean_seeds.print;
			});
		};
	});
}}
)

// click to place at least 5 centroids, then run this code for kmean

(
~ds = FluidDataSet(s).load(~fp.dict);

~kmeans = FluidKMeans(s).setMeans(~mean_seeds);
~labels = FluidLabelSet(s);
~kmeans.predict(~ds,~labels,{
	~labels.dump({
		arg labels;
		~fp.categories_(labels);
	});
});
)

// same with skmean

(
~ds = FluidDataSet(s).load(~fp.dict);

~skmeans = FluidSKMeans(s).setMeans(~mean_seeds);
~labels = FluidLabelSet(s);
~skmeans.predict(~ds,~labels,{
	~labels.dump({
		arg labels;
		~fp.categories_(labels);
	});
});
)

Comment on lines +227 to +228
assert(mDims != 0);
assert(mK != 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert(mDims != 0);
assert(mK != 0);
assert(mDims > 0);
assert(mK > 0);

if (!srcPtr) return Error(NoDataSet);
auto dataSet = srcPtr->getDataSet();
if (dataSet.size() == 0) return Error(EmptyDataSet);
if (dataSet.size() != get<kNumClusters>()) return Error(WrongNumInitial);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably the numClusters parameter can now be incorrect after setMeans. So this method actually needs to update the params and return these.

Something like (untested)

  MessageResult<ParamValues> setMeans(InputDataSetClientRef srcClient)
  {
    auto srcPtr = srcClient.get().lock();
    if (!srcPtr) return Error(NoDataSet);
    auto dataSet = srcPtr->getDataSet();
    if (dataSet.size() == 0) return Error(EmptyDataSet);
    auto numClusters = dataSet.size(); 
    mAlgorithm.setMeans(dataSet.getData()); 
    // update params and return to host
    ParamValues newParams = mParams.get().toTuple();
    std::get<kNumClusters>(newParams) = numClusters; 
    return newParams; 
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParamValues will need to be defined near the top of the class, like in the MLP objects:

using ParamValues = typename ParamSetViewType::ValueTuple;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FluidKMeans setMeans doesn't set numClusters

2 participants